refactor: make unmarshalling easier and override json to RawMessage #4145
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
ogzhanolguncu
changed the title
📝 WalkthroughWalkthroughAdds a generic DB helper UnmarshalNullableJSONTo[T any] and replaces scattered json.Unmarshal usages across key/identity handlers and tests with calls to this helper; updates minor local variable naming and groups a few type alias declarations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant DBpkg as db.UnmarshalNullableJSONTo
participant JSON as json.Unmarshal
Client->>Handler: Request (load key/identity)
Handler->>DBpkg: UnmarshalNullableJSONTo(data)
DBpkg->>JSON: if data is []byte|string -> json.Unmarshal into T
JSON-->>DBpkg: T or error
DBpkg-->>Handler: (T, nil) or (zero T, error)
Handler-->>Client: Response (meta set if unmarshal succeeded, else empty + logged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-08-08T15:09:01.312ZApplied to files:
📚 Learning: 2025-10-30T15:10:52.743ZApplied to files:
📚 Learning: 2025-08-08T15:09:01.312ZApplied to files:
📚 Learning: 2025-08-08T15:09:01.312ZApplied to files:
🧬 Code graph analysis (1)go/apps/api/routes/v2_keys_get_key/handler.go (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ogzhanolguncu
requested review from
Flo4604,
chronark,
imeyer and
perkinsjr
as code owners
|
Using json.rawmessage with inserts is a pain, that's why we are using []byte in the first place |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
go/pkg/db/custom_types.go(2 hunks)go/pkg/db/key_data.go(2 hunks)go/pkg/db/key_find_for_verification.sql_generated.go(2 hunks)go/pkg/db/key_find_live_by_hash.sql_generated.go(2 hunks)go/pkg/db/key_find_live_by_id.sql_generated.go(2 hunks)go/pkg/db/key_list_by_key_auth_id.sql_generated.go(2 hunks)go/pkg/db/key_list_live_by_auth_id.sql_generated.go(2 hunks)go/pkg/db/models_generated.go(4 hunks)go/pkg/db/sqlc.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
go/pkg/db/key_data.go (1)
go/pkg/db/custom_types.go (4)
UnmarshalJSONArray(51-66)RoleInfo(11-15)PermissionInfo(17-22)RatelimitInfo(24-32)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
go/pkg/db/models_generated.go (1)
Workspace(814-832)
go/pkg/db/key_list_by_key_auth_id.sql_generated.go (1)
go/pkg/db/models_generated.go (2)
Key(640-664)EncryptedKey(609-616)
go/pkg/db/key_find_live_by_id.sql_generated.go (1)
go/pkg/db/models_generated.go (4)
Api(514-525)KeyAuth(666-677)Workspace(814-832)EncryptedKey(609-616)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
🔇 Additional comments (18)
go/pkg/db/sqlc.json (1)
36-43: LGTM - Idiomatic JSON handling.The override mapping from JSON database type to
json.RawMessageis the idiomatic Go approach for representing JSON data. Whilejson.RawMessageis essentially[]byte, it explicitly signals JSON intent and integrates better with theencoding/jsonpackage.go/pkg/db/key_find_for_verification.sql_generated.go (2)
8-12: Generated code aligns with configuration.The addition of
encoding/jsonimport and the updatedIdentityMetafield type tojson.RawMessagecorrectly reflect the sqlc configuration override.
93-120: Type mapping correctly applied.The struct correctly uses
json.RawMessageforIdentityMeta(line 116) while aggregated fields likeRoles,Permissions, andRatelimitsremain asinterface{}since they're computed columns from subqueries, not actual database columns. This aligns with the PR's stated approach.go/pkg/db/key_find_live_by_id.sql_generated.go (2)
8-12: Consistent type handling.Import and type changes align with the project-wide JSON handling refactor.
106-142: Struct types correctly updated.The
IdentityMetafield now usesjson.RawMessage(line 135), consistent with the sqlc override. Aggregated relationship fields (Roles,Permissions,RolePermissions,Ratelimits) appropriately remain asinterface{}per the design constraints.go/pkg/db/key_list_by_key_auth_id.sql_generated.go (2)
8-12: Import added for JSON handling.Consistent with the JSON type refactor across generated files.
41-48: Type mapping applied correctly.The
IdentityMetafield usesjson.RawMessage(line 45), matching the global sqlc override for JSON columns.go/pkg/db/key_list_live_by_auth_id.sql_generated.go (2)
8-12: Import addition supports JSON handling.Consistent with other generated files in this refactor.
117-150: Field types correctly mapped.The
IdentityMetafield (line 143) correctly usesjson.RawMessage. Aggregated fields remaininterface{}as expected for computed JSON columns.go/pkg/db/key_find_live_by_hash.sql_generated.go (2)
8-12: JSON import added.Supports the
json.RawMessagetype used in the struct.
105-141: Type mappings consistent.The
IdentityMetafield (line 134) usesjson.RawMessage. Aggregated JSON columns appropriately remaininterface{}.go/pkg/db/custom_types.go (1)
3-7: Import added for JSON unmarshalling.The
encoding/jsonpackage supports the newUnmarshalJSONArrayhelper function.go/pkg/db/key_data.go (2)
84-88: Cleaner JSON array deserialization.Replacing nil initialization with
UnmarshalJSONArraycalls centralizes the unmarshalling logic and ensures consistent handling of JSON-aggregated fields. The helper always returns a valid (non-nil) slice, eliminating potential nil pointer issues.
134-138: Consistent pattern applied.Both
buildKeyDataandbuildKeyDataFromKeyAuthnow use the sameUnmarshalJSONArrayapproach for deserializing relationship fields, improving maintainability.go/pkg/db/models_generated.go (4)
555-567: Change from[]bytetojson.RawMessageforAuditLogTarget.Metais verified as safe.Downstream analysis found only three references to
AuditLogTargetin test code (ingo/apps/api/routes/v2_ratelimit_limit/200_test.go), accessing.Type,.ID, and.Name.Stringfields—not theMetafield. This confirms no breaking changes or required updates. Thejson.RawMessagetype is already established in the codebase as the standard pattern for JSON columns, making this change safe and correct.
629-638: LGTM! Type change verified across all query result structs.The change to
json.RawMessageforIdentity.Metais correct and consistently applied. All query result structIdentityMetafields have been updated to match (verified in 5 generated SQL query files), and all downstream usages in handlers and tests are compatible sincejson.RawMessageis a standard Go type alias for[]byte.
527-543: LGTM! No breaking changes found.The change from
[]bytetojson.RawMessageis compatible with all downstream code. The type flow is correct:
- Source
auditlog.AuditLog.ActorMetaismap[string]any- Converted via
json.Marshal()to[]byteininsert.go:69- Implicitly assigned to
db.AuditLog.ActorMetaasjson.RawMessage(which is defined astype RawMessage []byte)This is the idiomatic pattern for JSON database columns. All SQL-generated code and service code handle the field correctly without modification.
814-832: No breaking changes—field is unused in codebase.The
Subscriptionsfield is not referenced anywhere in the application code, tests, or migrations. Comprehensive searches across all Go files (excluding generated files) returned zero results for field access, struct initialization, or serialization patterns. The change from[]bytetojson.RawMessageis safe and idiomatic—both types are compatible at the database level, andjson.RawMessageis the Go standard for deferred JSON parsing. Since this is a sqlc-generated model file, the type change likely originates from an updated type mapping in the generation configuration, not manual code modification.Likely an incorrect or invalid review comment.
Wanna elaborate? |
|
See the error messages in the failing tests
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
That's not what I meant why don't we stick to that after fixing those errors? Whats the trade off of []byte vs raw.JsonMessage? Btw, I'm not saying you are wrong, but if its more idiomatic to use raw.Json thingy I say let's fix the issues and roll with this one |
|
ah I remember why we use []byte json.rawmessage doesn't support null JSON columns json.RawMessage does not support NULL · Issue #129 · sqlc-dev/sqlc So you will have to work around this |
|
@Flo4604 I'll check again if its really causing too much headache i'll revert it back to []byte and let you know 🫡 . Thanks for the explanation |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
103-118: Consider adding explicit NULL metadata test coverage.Given the PR's focus on improving NULL JSON column handling (see PR comments about
json.RawMessageNULL support issues), consider adding a test that creates an identity without metadata and explicitly verifies thatidentity.Metacan be safely unmarshaled or is properly represented as NULL/empty.Currently, this test creates an identity without metadata but doesn't verify the Meta field behavior.
Add a test case that verifies NULL metadata handling:
t.Run("create identity without metadata and verify NULL handling", func(t *testing.T) { externalTestID := uid.New("test_external_id") req := handler.Request{ExternalId: externalTestID} res := testutil.CallRoute[handler.Request, handler.Response](h, route, headers, req) require.Equal(t, 200, res.Status) identity, err := db.Query.FindIdentityByExternalID(ctx, h.DB.RO(), db.FindIdentityByExternalIDParams{ WorkspaceID: h.Resources().UserWorkspace.ID, ExternalID: externalTestID, Deleted: false, }) require.NoError(t, err) // Verify NULL or empty metadata is handled correctly dbMeta, err := db.UnmarshalNullableJSONTo[map[string]any](&identity.Meta) require.NoError(t, err) // Assert expected behavior for NULL/empty metadata // (adjust assertion based on your NullJSON implementation) })go/pkg/db/sqlc.json (1)
36-55: Consider a second override for non-nullable JSON → json.RawMessage.To avoid type fragmentation and keep non-nullable JSON fields strongly typed, add an override for nullable=false mapping to encoding/json.RawMessage.
Apply:
"overrides": [ { "db_type": "json", "go_type": { "type": "NullJSON", "package": "dbtype", "import": "github.com/unkeyed/unkey/go/pkg/db/types" }, "nullable": true }, + { + "db_type": "json", + "go_type": { + "type": "RawMessage", + "package": "json", + "import": "encoding/json" + }, + "nullable": false + }, { "column": "permissions.description", "go_type": { "type": "NullString", "package": "dbtype", "import": "github.com/unkeyed/unkey/go/pkg/db/types" }, "nullable": true } ]go/pkg/db/key_data.go (1)
84-87: LGTM! Cleaner JSON unmarshalling with centralized helpers.The refactoring replaces inline JSON unmarshalling with
UnmarshalJSONArrayTo, which provides consistent error handling (returning empty slices on failure) across all relationship fields. This improves maintainability and reduces code duplication.Also applies to: 134-137
go/apps/api/routes/v2_keys_verify_key/handler.go (1)
24-27: LGTM! Improved code organization.Grouping type aliases into a type block is more idiomatic and cleaner.
go/apps/api/routes/v2_identities_list_identities/handler.go (1)
19-22: LGTM! Improved code organization.Grouping type aliases is more idiomatic and consistent with other handlers in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
go/apps/api/routes/v2_apis_list_keys/handler.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/200_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/handler.go(2 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(1 hunks)go/apps/api/routes/v2_keys_verify_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(1 hunks)go/pkg/db/custom_types.go(2 hunks)go/pkg/db/identity_find_with_ratelimits.sql_generated.go(2 hunks)go/pkg/db/key_data.go(2 hunks)go/pkg/db/key_data_test.go(2 hunks)go/pkg/db/key_find_for_verification.sql_generated.go(2 hunks)go/pkg/db/key_find_live_by_hash.sql_generated.go(2 hunks)go/pkg/db/key_find_live_by_id.sql_generated.go(2 hunks)go/pkg/db/key_list_by_key_auth_id.sql_generated.go(2 hunks)go/pkg/db/key_list_live_by_auth_id.sql_generated.go(2 hunks)go/pkg/db/models_generated.go(4 hunks)go/pkg/db/sqlc.json(1 hunks)go/pkg/db/types/null_json.go(1 hunks)go/pkg/testutil/seed/seed.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- go/pkg/db/key_find_live_by_id.sql_generated.go
- go/pkg/db/key_list_by_key_auth_id.sql_generated.go
- go/pkg/db/custom_types.go
- go/pkg/db/key_list_live_by_auth_id.sql_generated.go
- go/pkg/db/models_generated.go
🧰 Additional context used
🧬 Code graph analysis (13)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)
go/pkg/db/key_data.go (1)
go/pkg/db/custom_types.go (4)
UnmarshalJSONArrayTo(51-66)RoleInfo(11-15)PermissionInfo(17-22)RatelimitInfo(24-32)
go/apps/api/routes/v2_keys_verify_key/handler.go (3)
go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)go/pkg/fault/wrap.go (1)
Wrap(25-67)
go/apps/api/routes/v2_apis_list_keys/handler.go (3)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)go/pkg/db/models_generated.go (1)
Identity(629-638)go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
go/apps/api/routes/v2_identities_list_identities/handler.go (2)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)
go/pkg/db/identity_find_with_ratelimits.sql_generated.go (1)
go/pkg/db/types/null_json.go (1)
NullJSON(10-13)
go/apps/api/routes/v2_identities_get_identity/handler.go (2)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)
go/pkg/db/key_data_test.go (2)
go/pkg/db/types/null_json.go (1)
NullJSON(10-13)go/pkg/db/models_generated.go (1)
Identity(629-638)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
go/pkg/db/types/null_json.go (1)
NullJSON(10-13)
go/apps/api/routes/v2_keys_get_key/handler.go (3)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)go/pkg/db/models_generated.go (1)
Identity(629-638)go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
go/pkg/testutil/seed/seed.go (3)
go/pkg/db/types/null_json.go (1)
NullJSON(10-13)go/pkg/db/identity_insert.sql_generated.go (1)
InsertIdentityParams(31-38)go/pkg/db/models_generated.go (2)
Environment(618-627)Identity(629-638)
go/apps/api/routes/v2_keys_whoami/handler.go (3)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-83)go/pkg/db/models_generated.go (1)
Identity(629-638)go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
go/pkg/db/key_find_for_verification.sql_generated.go (1)
go/pkg/db/types/null_json.go (1)
NullJSON(10-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
141-143: LGTM! Correct usage of the centralized JSON unmarshal helper.The replacement of direct
json.Unmarshalwithdb.UnmarshalNullableJSONTois correct and follows the new pattern introduced in this PR. Error handling is properly preserved.go/apps/api/routes/v2_identities_list_identities/200_test.go (1)
372-377: LGTM; aligns with NullJSON shape.Using Meta.Valid and comparing against NullJSON.Data with JSONEq is correct and order‑insensitive.
go/pkg/db/identity_find_with_ratelimits.sql_generated.go (1)
12-13: Verification complete—NullJSON integration is correct.The struct confirms Meta is
dbtype.NullJSON(line 73) and Scan properly handles NULL (returns Valid=false) and valid JSON (with a defensive buffer copy to prevent reuse corruption). Ratelimits correctly remainsinterface{}as a non-database field. All requirements are met.go/pkg/db/sqlc.json (1)
40-42: NullJSON properly implements Scanner/Valuer; no issues found.Verification confirms that
dbtype.NullJSONcorrectly implements bothdatabase/sql.Scanneranddriver.Valuerinterfaces with proper NULL handling. TheScan()method includes a defensive buffer copy to prevent corruption from driver buffer reuse—a critical safeguard. The sqlc override configuration is sound: nullable JSON columns map toNullJSONfor reads, while insert parameters usejson.RawMessagefor explicit values. This design is intentional and working correctly.go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
12-13: LGTM; NullJSON properly handles LEFT JOIN NULLs.The
NullJSONtype hasDataandValidfields, and itsScan()method correctly setsValid=falsewhen the database value is NULL. TheUnmarshalTo()method returnsnilwithout attempting to unmarshal when!Valid, preventing panics on NULL identity metadata from LEFT JOINs. All handlers safely useUnmarshalNullableJSONTo(), which wraps this behavior. The import change is correct.go/pkg/db/key_find_for_verification.sql_generated.go (1)
12-12: LGTM! Generated code reflects the new NullJSON type.The sqlc-generated code correctly imports the dbtype package and updates IdentityMeta to use dbtype.NullJSON, which properly handles nullable JSON columns.
Also applies to: 117-117
go/pkg/testutil/seed/seed.go (2)
380-380: LGTM! Naming convention improved.The rename from
identityIdtoidentityIDfollows Go naming conventions for acronyms, and all usages are correctly updated.Also applies to: 392-392, 397-397
372-387: Type inconsistency confirmed: verify if this is intended design.The verification confirms that
InsertIdentityParams.Metaisjson.RawMessagewhileIdentity.Metaisdbtype.NullJSON. Since both files are auto-generated, this pattern likely reflects intentional design: simpler raw types for insertion parameters and wrapped nullable types for model retrieval. However, the architectural intention needs confirmation—specifically whether the codebase applies this pattern consistently across other insert operations versus model definitions, and whether it introduces any conversion overhead or maintenance complexity that should be addressed at the code generator level.go/apps/api/routes/v2_keys_verify_key/handler.go (1)
232-238: LGTM! Consistent error handling with centralized helper.The refactoring replaces manual JSON unmarshalling with
db.UnmarshalNullableJSONTo, maintaining identical error handling behavior while improving code consistency across the codebase.go/pkg/db/types/null_json.go (3)
15-40: LGTM! Critical defensive copy prevents corruption.The defensive copy in the Scan method is essential to prevent data corruption from driver buffer reuse. This is a well-known issue with json.RawMessage and the implementation correctly addresses it with clear documentation.
42-47: LGTM! Correct SQL driver Value implementation.The Value method correctly returns
nilfor SQL NULL when the field is invalid, and properly converts the RawMessage to []byte when valid.
60-65: The silent no-op behavior is safe and works as intended.The
UnmarshalTomethod is only called through theUnmarshalNullableJSONTowrapper function (custom_types.go:79-83), which properly propagates the error to callers. All production usages check the returned error value. ReturningnilwhenValidisfalseis intentional and semantically correct: no error occurred, the field is null, and the caller receives a zero value—a reasonable default for nullable fields. The design properly distinguishes between actual unmarshaling errors (fromjson.Unmarshal) and null values.go/apps/api/routes/v2_identities_list_identities/handler.go (1)
139-146: LGTM! Consistent error handling pattern.The refactoring uses
db.UnmarshalNullableJSONTowith proper error wrapping. Unlike the list_keys handler which only logs errors, this returns the error to the caller, which is more appropriate for an API that lists identities where metadata is expected.go/pkg/db/key_data_test.go (1)
9-9: LGTM! Tests updated for new type.The test correctly uses the new
dbtype.NullJSONtype for IdentityMeta, ensuring the type change is properly validated.Also applies to: 125-125, 133-133
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
306-311: Behavior change is intentional and consistent across handlers.The old implementation checked both
len(keyData.Identity.Meta) > 0andidentityMeta != nilbefore assigning, while the new code assigns&identityMetawhenever unmarshaling succeeds. This means empty JSON objects{}now result in a pointer to an empty map instead of nil.However, this change is part of the broader refactor commit "refactor: use NullJSON for mysql json type" and follows the identical pattern already established in
v2_keys_whoamiandv2_keys_get_keyhandlers. The refactor also improves error handling by logging failures instead of silently ignoring them.Confirm whether this behavior change is acceptable for the API contract (nil vs. empty map in the response).
|
@Flo4604 I think I finally landed on a good solution, but there is some caveat // CRITICAL: A defensive copy to prevent buffer reuse corruption
// json.RawMessage holds a reference to the underlying byte slice.
// When the driver reuses that buffer for the next row, your NullJSON.Data suddenly points to corrupted memory.
// This consumes more memory compared to copyless version and also guarantees data integrity.
clone := make([]byte, len(buf))
copy(clone, buf)When scanning, I actually noticed this only during tests, data is getting corrupted due to how 
|
|
why don't we then just use our byte[] version and add a helper that does the same thing? |
|
|
|
I guess, yeah. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
go/apps/api/routes/v2_keys_whoami/handler.go (1)
158-162: Honor omitempty by checking data presence before unmarshalling and setting Meta.When
keyData.Identity.Metais nil or empty, the helper returns a zero-value map; taking&identityMetacreates a pointer to that nil map, which serializes as"meta": nullinstead of being omitted. Check for data presence first and only setresponse.Identity.Metawhen valid data exists.Apply the fix suggested in the previous review:
- if identityMeta, err := db.UnmarshalNullableJSONTo[map[string]any](keyData.Identity.Meta); err != nil { + if len(keyData.Identity.Meta) > 0 { + identityMeta, err := db.UnmarshalNullableJSONTo[map[string]any](keyData.Identity.Meta) + if err != nil { - h.Logger.Error("failed to unmarshal identity meta", "error", err) - } else { + h.Logger.Error("failed to unmarshal identity meta", "error", err) + } else { - response.Identity.Meta = &identityMeta + response.Identity.Meta = &identityMeta + } }go/apps/api/routes/v2_identities_get_identity/handler.go (1)
99-105: Log meta unmarshal errors instead of failing the request, and honor omitempty.Failing the entire request when identity metadata cannot be parsed degrades UX. Other endpoints (e.g.,
v2_keys_whoami) log the error and continue. Additionally, always settingMeta: &metaMapproduces"meta": nulleven when the DB column is empty or NULL; gate assignment on data presence to respect theomitemptytag.Apply the fix suggested in the previous review:
- metaMap, err := db.UnmarshalNullableJSONTo[map[string]any](identity.Meta) - if err != nil { - return fault.Wrap(err, - fault.Internal("unable to unmarshal metadata"), - fault.Public("We're unable to parse the identity's metadata."), - ) - } + var metaPtr *map[string]any + if len(identity.Meta) > 0 { + metaMap, err := db.UnmarshalNullableJSONTo[map[string]any](identity.Meta) + if err != nil { + h.Logger.Error("failed to unmarshal identity meta", "error", err) + } else { + metaPtr = &metaMap + } + }And update the response assignment:
Data: openapi.Identity{ Id: identity.ID, ExternalId: identity.ExternalID, - Meta: &metaMap, + Meta: metaPtr, Ratelimits: &responseRatelimits, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
go/apps/api/routes/v2_apis_list_keys/handler.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/handler.go(2 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(1 hunks)go/apps/api/routes/v2_keys_verify_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(1 hunks)go/pkg/db/custom_types.go(2 hunks)go/pkg/testutil/seed/seed.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- go/pkg/testutil/seed/seed.go
- go/apps/api/routes/v2_apis_list_keys/handler.go
- go/pkg/db/custom_types.go
- go/apps/api/routes/v2_keys_get_key/handler.go
🧰 Additional context used
🧬 Code graph analysis (5)
go/apps/api/routes/v2_keys_verify_key/handler.go (4)
go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)go/pkg/db/models_generated.go (1)
Identity(629-638)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-94)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)
go/apps/api/routes/v2_keys_whoami/handler.go (3)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-94)go/pkg/db/models_generated.go (1)
Identity(629-638)go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-94)
go/apps/api/routes/v2_identities_list_identities/handler.go (3)
go/apps/api/openapi/gen.go (3)
V2IdentitiesListIdentitiesRequestBody(708-714)V2IdentitiesListIdentitiesResponseBody(717-726)Meta(279-282)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-94)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)
go/apps/api/routes/v2_identities_get_identity/handler.go (2)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(79-94)go/pkg/fault/wrap.go (3)
Wrap(25-67)Internal(75-89)Public(97-111)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
🔇 Additional comments (3)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
141-141: LGTM!The test code correctly uses the new unmarshalling helper. The pattern is consistent across all test cases and appropriately validates metadata handling.
Also applies to: 241-241, 316-316, 425-425
go/apps/api/routes/v2_keys_verify_key/handler.go (1)
24-27: LGTM!Grouping related type aliases in a single
typeblock is idiomatic Go and improves readability.go/apps/api/routes/v2_identities_list_identities/handler.go (1)
19-22: LGTM!Grouping related type aliases in a single
typeblock is idiomatic Go and improves readability.
|
yeah also we shouldn't fail when json parsing the meta object, as we discussed in slack the object should be valid since its validated as object when sending via the API and the frontend also validates it afaik. |
Yeah I didn't change the existing behavior. We just log errors move on |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
go/apps/api/routes/v2_identities_get_identity/handler.go (1)
99-121: Honor omitempty for Meta; set pointer only when meta is present.Current code always sets Meta: &metaMap, yielding
"meta": nullwhen absent/invalid. Gate it so Meta is omitted unless parsed.- metaMap := db.UnmarshalNullableJSONTo[map[string]any](identity.Meta, h.Logger) + var metaMap map[string]any + if identity.Meta != nil { // or check specific NullString/Valid shape if applicable + metaMap = db.UnmarshalNullableJSONTo[map[string]any](identity.Meta, h.Logger) + } @@ - Meta: &metaMap, + Meta: func() *map[string]any { if metaMap == nil { return nil }; return &metaMap }(),Optional: also use the same helper for ratelimits for consistency/logging.
- var ratelimits []db.RatelimitInfo - if ratelimitBytes, ok := identity.Ratelimits.([]byte); ok && ratelimitBytes != nil { - _ = json.Unmarshal(ratelimitBytes, &ratelimits) - } + ratelimits := db.UnmarshalNullableJSONTo[[]db.RatelimitInfo](identity.Ratelimits, h.Logger)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
go/apps/api/routes/v2_apis_list_keys/handler.go(2 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/handler.go(2 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_reroll_key/200_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/handler.go(1 hunks)go/apps/api/routes/v2_keys_verify_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(2 hunks)go/pkg/db/custom_types.go(2 hunks)go/pkg/db/key_data.go(5 hunks)go/pkg/db/key_data_test.go(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go/apps/api/routes/v2_apis_list_keys/handler.go
- go/apps/api/routes/v2_keys_get_key/handler.go
🧰 Additional context used
🧬 Code graph analysis (9)
go/apps/api/routes/v2_keys_verify_key/handler.go (3)
go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)go/pkg/db/models_generated.go (1)
Identity(629-638)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(57-86)
go/apps/api/routes/v2_keys_reroll_key/200_test.go (1)
go/pkg/db/key_data.go (1)
ToKeyData(30-47)
go/pkg/db/key_data.go (4)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
FindLiveKeyByHashRow(104-140)go/pkg/db/key_find_live_by_id.sql_generated.go (1)
FindLiveKeyByIDRow(105-141)go/pkg/db/key_list_live_by_auth_id.sql_generated.go (1)
ListLiveKeysByKeyAuthIDRow(116-149)go/pkg/db/custom_types.go (4)
UnmarshalNullableJSONTo(57-86)RoleInfo(13-17)PermissionInfo(19-24)RatelimitInfo(26-34)
go/apps/api/routes/v2_identities_get_identity/handler.go (1)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(57-86)
go/pkg/db/key_data_test.go (1)
go/pkg/db/key_data.go (1)
ToKeyData(30-47)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(57-86)
go/apps/api/routes/v2_identities_list_identities/handler.go (2)
go/apps/api/openapi/gen.go (3)
V2IdentitiesListIdentitiesRequestBody(708-714)V2IdentitiesListIdentitiesResponseBody(717-726)Meta(279-282)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(57-86)
go/apps/api/routes/v2_keys_reroll_key/handler.go (1)
go/pkg/db/key_data.go (1)
ToKeyData(30-47)
go/apps/api/routes/v2_keys_whoami/handler.go (4)
go/pkg/db/key_data.go (1)
ToKeyData(30-47)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(57-86)go/pkg/db/models_generated.go (1)
Identity(629-638)go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
🔇 Additional comments (1)
go/apps/api/routes/v2_keys_reroll_key/handler.go (1)
82-82: LGTM: pass logger into ToKeyData.Signature updated correctly; aligns with helper-based JSON handling.
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
is the logger really necessary?
@chronark Can we just log it with regular log instead of our logger package. That would make everything super clean? |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
go/apps/api/routes/v2_keys_get_key/handler.go (1)
166-173: Check data validity and avoid emitting empty meta objects.The code unmarshals
keyData.Identity.Metawithout first checking if data is present, and always assignsresponse.Identity.Meta = &identityMetaeven when unmarshalling fails (resulting in an empty map). This produces"meta": {}instead of omitting the field per theomitemptytag.Apply this diff to honor
omitemptyand align with other endpoints:- if identityMeta, err := db.UnmarshalNullableJSONTo[map[string]any](keyData.Identity.Meta); err != nil { + if len(keyData.Identity.Meta) > 0 { + identityMeta, err := db.UnmarshalNullableJSONTo[map[string]any](keyData.Identity.Meta) + if err != nil { - h.Logger.Error("failed to unmarshal identity meta", - "identityId", keyData.Identity.ID, - "error", err, - ) - } else { - response.Identity.Meta = &identityMeta + h.Logger.Error("failed to unmarshal identity meta", + "identityId", keyData.Identity.ID, + "error", err, + ) + } else if len(identityMeta) > 0 { + response.Identity.Meta = &identityMeta + } }go/apps/api/routes/v2_identities_list_identities/handler.go (1)
139-148: Only assign Meta when map is non-empty to honor omitempty.Line 147 assigns
newIdentity.Meta = &metaMapeven whenmetaMapis an empty map (from nil/empty DB data), producing"meta": {}for identities with no metadata instead of omitting the field per theomitemptytag.Apply:
metaMap, err := db.UnmarshalNullableJSONTo[map[string]any](identity.Meta) if err != nil { h.Logger.Error("failed to unmarshal identity meta", "identityId", identity.ID, "error", err, ) // Continue with empty meta - } else { + } else if len(metaMap) > 0 { newIdentity.Meta = &metaMap }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
go/apps/api/routes/v2_apis_list_keys/handler.go(2 hunks)go/apps/api/routes/v2_identities_create_identity/200_test.go(4 hunks)go/apps/api/routes/v2_identities_get_identity/handler.go(2 hunks)go/apps/api/routes/v2_identities_list_identities/handler.go(2 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_reroll_key/handler.go(0 hunks)go/apps/api/routes/v2_keys_verify_key/handler.go(3 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(2 hunks)go/pkg/db/custom_types.go(2 hunks)go/pkg/db/key_data.go(2 hunks)
💤 Files with no reviewable changes (1)
- go/apps/api/routes/v2_keys_reroll_key/handler.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go/apps/api/routes/v2_keys_whoami/handler.go
- go/apps/api/routes/v2_apis_list_keys/handler.go
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/pkg/db/querier_generated.go:387-403
Timestamp: 2025-08-14T18:31:49.604Z
Learning: In MySQL's JSON_OBJECT function, boolean expressions like `rl.auto_apply = 1` automatically convert to proper JSON boolean values (true/false), not numeric values (0/1). This means Go's json.Unmarshal can correctly handle these fields when unmarshalling into bool types without any conversion issues.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: For go/apps/api/openapi, oapi-codegen is used and does not support OpenAPI 3.1 union types like [T, "null"]; an overlay step is required to downconvert to 3.0-style nullable before code generation.
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3297
File: apps/dashboard/lib/trpc/routers/authorization/roles/query.ts:210-323
Timestamp: 2025-06-04T20:13:12.060Z
Learning: The user ogzhanolguncu prefers explicit, duplicated code over abstracted helper functions when it improves readability, even if it means some duplication in filter building functions in the authorization roles query module.
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2825
File: apps/dashboard/app/(app)/logs-v2/hooks/use-bookmarked-filters.ts:0-0
Timestamp: 2025-01-30T20:51:44.359Z
Learning: The user (ogzhanolguncu) prefers to handle refactoring suggestions in separate PRs to maintain focus in the current PR.
📚 Learning: 2025-10-30T15:10:52.743Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
go/apps/api/routes/v2_identities_get_identity/handler.gogo/apps/api/routes/v2_keys_verify_key/handler.gogo/pkg/db/key_data.gogo/apps/api/routes/v2_keys_get_key/handler.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: For go/apps/api/openapi, oapi-codegen is used and does not support OpenAPI 3.1 union types like [T, "null"]; an overlay step is required to downconvert to 3.0-style nullable before code generation.
Applied to files:
go/apps/api/routes/v2_identities_get_identity/handler.gogo/pkg/db/custom_types.gogo/apps/api/routes/v2_keys_verify_key/handler.gogo/pkg/db/key_data.gogo/apps/api/routes/v2_keys_get_key/handler.go
📚 Learning: 2025-08-21T15:54:45.198Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3825
File: go/internal/services/usagelimiter/limit.go:38-0
Timestamp: 2025-08-21T15:54:45.198Z
Learning: In go/internal/services/usagelimiter/limit.go, the UpdateKeyCreditsDecrement operation cannot be safely wrapped with db.WithRetry due to the lack of idempotency mechanisms in the current tech stack. Retrying this non-idempotent write operation risks double-charging users if the first attempt commits but the client sees a transient error.
Applied to files:
go/apps/api/routes/v2_identities_get_identity/handler.go
📚 Learning: 2025-07-16T15:38:53.491Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.491Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.
Applied to files:
go/pkg/db/custom_types.go
📚 Learning: 2025-08-14T18:31:49.604Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/pkg/db/querier_generated.go:387-403
Timestamp: 2025-08-14T18:31:49.604Z
Learning: In MySQL's JSON_OBJECT function, boolean expressions like `rl.auto_apply = 1` automatically convert to proper JSON boolean values (true/false), not numeric values (0/1). This means Go's json.Unmarshal can correctly handle these fields when unmarshalling into bool types without any conversion issues.
Applied to files:
go/pkg/db/custom_types.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, oapi-codegen doesn’t support OpenAPI 3.1 union nullability; overlay.yaml must be applied before codegen. The overlay key in oapi-codegen config isn’t supported—use a pre-step (programmatic or CLI) to merge overlay into the bundled spec, then run oapi-codegen.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/handler.gogo/pkg/db/key_data.gogo/apps/api/routes/v2_keys_get_key/handler.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, overlays are required to downconvert 3.1 union nullability to 3.0-style nullable before running oapi-codegen; config.yaml’s output-options.overlay is not recognized by oapi-codegen, so overlays must be applied in a pre-step (programmatic or CLI) prior to codegen.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/handler.gogo/pkg/db/key_data.gogo/apps/api/routes/v2_keys_get_key/handler.go
📚 Learning: 2025-08-27T14:08:31.731Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/apps/api/routes/v2_keys_migrate_keys/handler.go:60-65
Timestamp: 2025-08-27T14:08:31.731Z
Learning: In the Unkey Go codebase, service methods like GetRootKey that return an emit function alongside potential errors are designed to always return a safe no-op emit function (emptyLog), even when the main operation fails. This makes it safe to defer emit() immediately after the call, before checking the error. This pattern is consistently used across all API route handlers.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/handler.go
📚 Learning: 2025-08-08T16:10:00.224Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/handler.go
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/handler.go
📚 Learning: 2025-08-08T15:10:46.436Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/handler.go
📚 Learning: 2025-08-08T14:59:52.283Z
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/handler.go
📚 Learning: 2025-07-15T14:59:30.212Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3560
File: go/deploy/metald/internal/database/repository.go:0-0
Timestamp: 2025-07-15T14:59:30.212Z
Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.
Applied to files:
go/apps/api/routes/v2_identities_create_identity/200_test.go
🧬 Code graph analysis (6)
go/apps/api/routes/v2_identities_get_identity/handler.go (1)
go/pkg/db/custom_types.go (2)
UnmarshalNullableJSONTo(54-75)RatelimitInfo(25-33)
go/apps/api/routes/v2_identities_list_identities/handler.go (2)
go/apps/api/openapi/gen.go (3)
V2IdentitiesListIdentitiesRequestBody(708-714)V2IdentitiesListIdentitiesResponseBody(717-726)Meta(279-282)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(54-75)
go/apps/api/routes/v2_keys_verify_key/handler.go (3)
go/apps/api/openapi/gen.go (2)
Meta(279-282)Identity(161-173)go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(54-75)go/pkg/db/models_generated.go (2)
Key(640-664)Identity(629-638)
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(54-75)
go/pkg/db/key_data.go (1)
go/pkg/db/custom_types.go (4)
UnmarshalNullableJSONTo(54-75)RoleInfo(12-16)PermissionInfo(18-23)RatelimitInfo(25-33)
go/apps/api/routes/v2_keys_get_key/handler.go (3)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(54-75)go/pkg/db/models_generated.go (2)
Identity(629-638)Key(640-664)go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: autofix
🔇 Additional comments (2)
go/pkg/db/custom_types.go (1)
35-74: LGTM! Clean implementation of nullable JSON unmarshalling.The function correctly handles all edge cases (nil data, empty bytes, type assertion failures, unmarshal errors) and returns descriptive errors for each failure mode. The generic signature with error return allows callers to decide how to handle failures appropriately.
go/apps/api/routes/v2_identities_create_identity/200_test.go (1)
141-143: LGTM! Correct usage of the unmarshalling helper in tests.All test cases properly call
UnmarshalNullableJSONTo, check the returned error withrequire.NoError, and use the result for assertions. The pattern is consistent and correct.Also applies to: 241-243, 316-318, 425-427
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
go/apps/api/routes/v2_keys_get_key/handler.go (1)
250-258: Only assign Meta when map is non-empty to respect omitempty.The code assigns
response.Meta = &metaeven when the unmarshalled map is empty, resulting in"meta": {}in the response. This issue has already been flagged in a previous review comment with the correct fix.
🧹 Nitpick comments (1)
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
319-322: LGTM – Clean refactoring to centralized helper.The migration to
db.UnmarshalNullableJSONTocorrectly handles nil/empty checks and follows the agreed log-and-continue pattern for meta parsing failures.Minor enhancement: consider adding the identity ID to the error log for consistency with the key meta error logging at line 391, which includes
keyId. This would improve debugging context.- if identityMeta, err := db.UnmarshalNullableJSONTo[map[string]any](keyData.Identity.Meta); err != nil { - h.Logger.Error("failed to unmarshal identity meta", "error", err) + if identityMeta, err := db.UnmarshalNullableJSONTo[map[string]any](keyData.Identity.Meta); err != nil { + h.Logger.Error("failed to unmarshal identity meta", + "identityId", keyData.Identity.ID, + "error", err, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
go/apps/api/routes/v2_apis_list_keys/handler.go(2 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(2 hunks)go/apps/api/routes/v2_keys_whoami/handler.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go/apps/api/routes/v2_keys_whoami/handler.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 3297
File: apps/dashboard/lib/trpc/routers/authorization/roles/query.ts:210-323
Timestamp: 2025-06-04T20:13:12.060Z
Learning: The user ogzhanolguncu prefers explicit, duplicated code over abstracted helper functions when it improves readability, even if it means some duplication in filter building functions in the authorization roles query module.
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2825
File: apps/dashboard/app/(app)/logs-v2/hooks/use-bookmarked-filters.ts:0-0
Timestamp: 2025-01-30T20:51:44.359Z
Learning: The user (ogzhanolguncu) prefers to handle refactoring suggestions in separate PRs to maintain focus in the current PR.
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: For go/apps/api/openapi, oapi-codegen is used and does not support OpenAPI 3.1 union types like [T, "null"]; an overlay step is required to downconvert to 3.0-style nullable before code generation.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.gogo/apps/api/routes/v2_keys_get_key/handler.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, overlays are required to downconvert 3.1 union nullability to 3.0-style nullable before running oapi-codegen; config.yaml’s output-options.overlay is not recognized by oapi-codegen, so overlays must be applied in a pre-step (programmatic or CLI) prior to codegen.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.gogo/apps/api/routes/v2_keys_get_key/handler.go
📚 Learning: 2025-08-08T15:09:01.312Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:09:01.312Z
Learning: Repo unkeyed/unkey: In go/apps/api/openapi, oapi-codegen doesn’t support OpenAPI 3.1 union nullability; overlay.yaml must be applied before codegen. The overlay key in oapi-codegen config isn’t supported—use a pre-step (programmatic or CLI) to merge overlay into the bundled spec, then run oapi-codegen.
Applied to files:
go/apps/api/routes/v2_apis_list_keys/handler.gogo/apps/api/routes/v2_keys_get_key/handler.go
📚 Learning: 2025-10-30T15:10:52.743Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
go/apps/api/routes/v2_keys_get_key/handler.go
🧬 Code graph analysis (2)
go/apps/api/routes/v2_apis_list_keys/handler.go (3)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(54-80)go/apps/api/openapi/gen.go (2)
Identity(161-173)Meta(279-282)go/pkg/db/models_generated.go (2)
Identity(643-652)Key(654-678)
go/apps/api/routes/v2_keys_get_key/handler.go (2)
go/pkg/db/custom_types.go (1)
UnmarshalNullableJSONTo(54-80)go/pkg/db/models_generated.go (2)
Identity(643-652)Key(654-678)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
go/apps/api/routes/v2_apis_list_keys/handler.go (1)
388-395: LGTM – Proper error context and type handling.The refactoring correctly handles
sql.NullStringby checkingValidbefore accessingString, and the error log includeskeyIdfor effective debugging. The helper seamlessly handles the string-to-bytes conversion needed for JSON unmarshalling.
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (11/06/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
What does this PR do?
This PR simplifies casting aggregated json data to concrete types. Also, replaces
[]byteoverride withjson.RawMessagebecause it represents our JSON Db columns better. Regarding dynamically castinginterface{}to associated type, I tried to override them insqlc.json, but since those values are aggregated and not really a column in the db, override fails. I also tried to do plugin but cons are overweighed the pros and finally I tried to do replace them using a script then I ended up with too much magic and very error prone code.UnmarshalJSONArraythis is the most idiomatic way to solve this.https://linear.app/unkey/issue/ENG-1991/improve-type-safety-for-json-columns-in-database-structs
Fixes #3789
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated